3398 - FRA Reports - search and upload #3445
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3445 +/- ##
===========================================
+ Coverage 91.15% 91.18% +0.02%
===========================================
Files 308 316 +8
Lines 8832 9120 +288
Branches 670 711 +41
===========================================
+ Hits 8051 8316 +265
- Misses 654 667 +13
- Partials 127 137 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
jtimpe
requested review from
andrew-jameson,
raftmsohani,
elipe17,
reitermb and
victoriaatraft
| ref={errorsRef} | ||
| tabIndex="-1" | ||
| > | ||
| There {errorsCount === 1 ? 'is' : 'are'} {form.errors} error(s) in |
There was a problem hiding this comment.
I noticed the form error count doesn't update as errors are resolved which is different than the datafiles page.
| 'We can’t process that file format. Please provide a plain text file.' | ||
|
|
||
| const INVALID_EXT_ERROR = | ||
| 'Invalid extension. Accepted file types are: .txt, .ms##, .ts##, or .ts###.' |
There was a problem hiding this comment.
I think FRA is only supposed to be .csv and .xlsx. We should verify
There was a problem hiding this comment.
@victoriaatraft @reitermb @ADPennington could you confirm?
| return ( | ||
| <> | ||
| <div className={classNames({ 'border-bottom': isUploadReportToggled })}> | ||
| <SearchForm |
There was a problem hiding this comment.
If you cause a form error in the UploadForm it does not get reset if you re-search using the SearchForm.
| <fieldset className="usa-fieldset"> | ||
| <legend className="usa-label text-bold">File Type</legend> | ||
|
|
||
| {options.map(({ label, value }, index) => ( |
There was a problem hiding this comment.
We use this pattern for radio selecters in the request access page too. Should make the radio controls a generic "use everywhere" component?
There was a problem hiding this comment.
this is captured by another ticket - #3015 - but was a simple enough change (that we keep kicking around because it's low priority) so i went for it. I also made a DropdownSelect component that handles the year/quarter inputs, since those looked very similar. I tried to structure them in a way that we could go implement a Form higher-order component (or context component) and use these components within it.
one caveat is that each input still needs to be wrapped in a div to add the correct styling. e.g.,
<div
className={classNames('usa-form-group maxw-mobile margin-top-4', {
'usa-form-group--error': !valid,
})}
>
<DropdownSelect ... />
</div>This is partly necessary (form-group) partly stylistic (margin-top-4) and partly-logical (usa-form-group--error if invalid), so it didn't fit neatly into either the reusable input component or the form-specific component. To solve that, I added a classes prop where the form-specific (such as margins) styling can be added conditionally from the wrapping component, if desired. I think we use margin-top-4 everywhere, but this adds flexibility should we decide to change something in the future.
The other bit of weirdness is the default/disabled/hidden first option in DropdownSelect. I decided to key off value === '' to decide if it's the disabled option, rather than add a disabled: true key to the options array or passing in a separate disabledOption="xyz" prop to the component.
Open to suggestions on all of this! I realize both those design decisions may be confusing to read and implement
| description = | ||
| 'The Work Outcomes of TANF Exiters report contains the Social Security Numbers (SSNs) of all work-eligible individuals who exit TANF in a given quarter and the dates in YYYYMM format that each individual exited TANF.' | ||
| aboutLink = '' | ||
| templateLink = '' |
There was a problem hiding this comment.
waiting on the final links from UX!
The other options were disabled per the ticket AC - I had glossed over this line initially, so they were enabled the first time you saw it |
| handlePreview, | ||
| getTargetClassName, | ||
| tryGetUTF8EncodedFile, | ||
| } from './utils' | ||
|
|
||
| const INVALID_FILE_ERROR = |
There was a problem hiding this comment.
The file upload on FRA still expects standard datafiles.
There was a problem hiding this comment.
Found a form bug on this one too. Not sure if it was pre-existing or not. Upload a file that causes the file extension error seen above. Change search parameters, search, see that extension error is still rendered.
There was a problem hiding this comment.
Tried duplicating these on the TANF data files page and it seems to be allowing any file type to be uploaded.
There was a problem hiding this comment.
on the old data files page, we noticed that the extension errors are not being displayed. unsure exactly why, but it allows submission until the nginx file check catches it
There was a problem hiding this comment.
fixed allowed extensions and the persistent error bug. good catches, thank you!
There was a problem hiding this comment.
WRT the datafiles page, are we spinning off a new ticket for that?
| onError(error) | ||
| } | ||
|
|
||
| dispatch({ |
There was a problem hiding this comment.
Should we stick this in a finally block just for safety? Looks like onError just returns null for the moment so it might not be necessary.
| let errors = 0 | ||
| Object.keys(newFormState).forEach((key) => { | ||
| if (key !== 'errors') { | ||
| if (newFormState[key].touched && !newFormState[key].valid) { |
There was a problem hiding this comment.
nit: can we combine these conditions to one statement?
| return | ||
| } | ||
|
|
||
| const encodedFile = await tryGetUTF8EncodedFile( |
There was a problem hiding this comment.
This should only be used on FRA files iff it is a csv file. If we do this to a .xlsx it will be corrupted and unreadable on the backend.
|
I am getting circular import error. |
It is fixed now |
|
options for FRA file types are grayed out |
This is correct per the ticket AC. covered here - #3445 (comment) |
| @@ -14,7 +13,11 @@ import { | |||
| } from '../../actions/reports' | |||
| import Button from '../Button' | |||
| import createFileInputErrorState from '../../utils/createFileInputErrorState' | |||
| import { handlePreview, getTargetClassName } from './utils' | |||
There was a problem hiding this comment.
'Cancel' button in the file submit form: makes the form to disappear. I think it should only clear the form and not delete it.
There was a problem hiding this comment.
@reitermb @victoriaatraft hiding the form on Cancel is consistent with the other data files page, but Mo brings up a good point. thoughts?
tdrs-backend/tdpservice/conftest.py
Outdated
| 'file': create_temporary_file(fake_file, 'report.csv'), | ||
| "original_filename": 'report.csv', | ||
| "slug": str(uuid.uuid4()), | ||
| "extension": "txt", |
There was a problem hiding this comment.
Also, should the section and group be different?
| } from '../../selectors/auth' | ||
|
|
||
| const isAllowed = ( | ||
| { permissions, isApproved }, | ||
| { permissions, isApproved, featureFlags }, |
There was a problem hiding this comment.
Should this have an override to all Sys admin and maybe developer groups to see everything? I was a little confused when I logged in as a Sys admin and could not see the FRA tab by default.
There was a problem hiding this comment.
This could certainly be follow on work. Or we can just specifiy that feature flags can be mutually exclusive from group assignments.
There was a problem hiding this comment.
good point. i personally prefer the explicit/mutually-exclusive approach but i'll let @ADPennington make the final call
Summary of Changes
Pull request closes #3398
TODO:implements search and upload backend endpoints -requires Add FRA report types to DataFile model and implement reparsing logic #3397TODO:implements FRA page permissions and feature flag -requires Implement feature flag for controlled access to FRA Data Files page #3399TODO:implement file encoding validation/conversion -requires Updated File Upload to Auto Encode to UTF-8 #3438How to Test
{"fra_reports": true}to user's feature flags to enable the page. Navigate to the FRA Reports page(TODO), uploads should succeedfra_reportsflag to various values. should work only when=== trueDeliverables
More details on how deliverables herein are assessed included here.
Deliverable 1: Accepted Features
Checklist of ACs:
lfrohlichand/oradpenningtonconfirmed that ACs are met.Deliverable 2: Tested Code
CodeCov Reportcomment in PR)CodeCov Reportcomment in PR)Deliverable 3: Properly Styled Code
Deliverable 4: Accessible
iamjollyandttran-hubusing Accessibility Insights reveal any errors introduced in this PR?Deliverable 5: Deployed
Deliverable 6: Documented
Deliverable 7: Secure
Deliverable 8: User Research
Research product(s) clearly articulate(s):